Fix unfinished step in parallel flow #4567

doontagi · 2024-03-24T14:18:18Z

related issue #3939

Motivation

On Different behavior when interrupting a job, depending on the parallel flow order #3939, when two flows are executed in parallel, I found that even though one flow throw an interruption and terminated the job, the next step of the other flow was executed.
And this abnormal behavior occurs only when the other flow was added secondly to the job
Problem is that even if the job containing both flows were terminated and went into STOPPED status, the other flow couldn't recognize the interruption because interruption had been checked by STOPPING status.
https://github.com/spring-projects/spring-batch/blob/9a2b1bbc96f338017aaa5df51214896f5a32d1e5/spring-batch-core/src/main/java/org/springframework/batch/core/repository/support/SimpleJobRepository.java#L290C16-L290C17
Because the job already had been terminated and in STOPPED status, a step in the other flow cannot recognize it and proceeded as like there had been no interruption.
The reason why this behavior didn't occur when the interruptedFlow added secondly to the job is that secondly added flow was always executed secondly in SplitState. So, if the two step flow would be added firstly, first step in this flow will be processed before the interrupted flow made the job status STOPPED, so it can recognize the interruption when one step has been executed.
If there are remaining tasks (steps) but the job was terminated due to an exception as like now, there is also a problem that the status of the job may be changed by the remaining tasks.

Modification

Changed to wait for all tasks to finish before throwing an exception.
Changed a job to handle exceptions after waiting for tasks currently being processed to complete. Prevent the job from terminating �even though there are remaining tasks.

Result

Job, Flow and Steps work same when an exception does not occur.
Regardless of the order in which flows are added to the Job for parallel processing, if an interruption occurs, only the currently executing step is processed. That is, no further steps are executed after that.
Close Different behavior when interrupting a job, depending on the parallel flow order #3939

doontagi · 2024-03-27T10:47:14Z

...tch-core/src/main/java/org/springframework/batch/core/job/flow/support/state/SplitState.java

 				}
 			}
 		}

+		if (!exceptions.isEmpty()) {
+			throw exceptions.get(0);


As before, the first caught exception would be thrown.

doontagi · 2024-03-27T10:49:39Z

...batch-core/src/test/java/org/springframework/batch/core/job/builder/FlowJobBuilderTests.java

+		jobBuilder.preventRestart().build().execute(execution);
+
+		boolean isExecutedNonExecutableStep = countDownLatch.await(1, TimeUnit.SECONDS);
+		assertEquals(BatchStatus.STOPPED, execution.getStatus());


Finished with BatchStatus.STOPPED without executing nonExecutableStep.
This test would fail with executing nonExecutableStep if it runs on the current main branch.

doontagi · 2024-03-27T10:51:18Z

Gentle ping to @fmbenhassine. Could you review this PR? I've fixed #3939.
Thanks!

fmbenhassine · 2024-04-23T10:35:08Z

Thank you for your PR!

Changed to wait for all asynchronous tasks to finish before throwing an exception.

Isn't that not desired when one of the tasks is interrupted? Meaning if one wants to interrupt a step, the idea is not to wait for other steps to finish.

The change in this PR fixes the reported issue, I just want to make sure there is no side effect on the behaviour of step interruption.

doontagi · 2024-07-14T09:40:56Z

Thank you for the comment! And sorry for the late reply. @fmbenhassine

I've also thought about your concern. And I don't think this change of exception handling location affects the current process of handling interruptions in steps, so has no side effect.
Because even if an interruption occurs in one of the steps being processed at the same time, the other step may recognize the interruption only after its execution is finished.

spring-batch/spring-batch-core/src/main/java/org/springframework/batch/core/step/AbstractStep.java

Line 230 in 425134c

doExecute(stepExecution);

Interruption only can be recognized in catch or final blocks after doExecute has been done.

Therefore, even if interrupt occurs in a step, other steps process its executions until the end and check interruptions from other steps regardless of my modification. In the virtue of my modification, other steps can recognize the interruptions because job is not STOPPED yet.

The difference is that the current code immediately propagates the exception generated by interrupt and terminates the job (STOPPED) without caring of other steps. Therefore, the remaining flows and steps cannot recognize the interrupt because it recognize the interrupt by STOPPING status of the job

spring-batch/spring-batch-core/src/main/java/org/springframework/batch/core/repository/support/SimpleJobRepository.java

Lines 287 to 294 in 425134c

    
           private void checkForInterruption(StepExecution stepExecution) { 
        
           	JobExecution jobExecution = stepExecution.getJobExecution(); 
        
           	jobExecutionDao.synchronizeStatus(jobExecution); 
        
           	if (jobExecution.isStopping()) { 
        
           		logger.info("Parent JobExecution is stopped, so passing message on to StepExecution"); 
        
           		stepExecution.setTerminateOnly(); 
        
           	} 
        
           }

and continue to be executed with STOPPED status of their parent job. It is a cause of the bug.

In conclusion, there is no change from the view of steps, because in the current code the step execution is also not stopped even if an exception caused by interrupt occurs in another step. Interrupt is caught after step execution has been done. And I think Job shouldn't be stopped if there are running flows and steps.

So I think my changes will prevent other unknown bugs that can occur as flow and step continue to run with job stopped.

Sorry for the lengthy explanation. if you have any other opinions, please leave a comment for more discussions. Thank you!

doontagi · 2024-12-16T09:36:45Z

Gentle remind @fmbenhassine. Could you check this PR?
Thanks!

fmbenhassine · 2024-12-17T04:58:30Z

Thank you for your feedback! I saw your comment back then and added a thumbs-up emoji and the status: feedback-provided tag to acknowledge your update. I should have mentioned the release in which i was planning to include this to avoid you waiting for an update from my side. Anyway, this looks good to me and should go into 5.2.1 planned for tomorrow.

Thank you for going into details with your last update! I love how you explained the step interruption process and how this change should not have a side effect 👍

fmbenhassine · 2024-12-17T05:01:44Z

Rebased and merged as 7678949. Thank you for your contribution!

doontagi mentioned this pull request Mar 24, 2024

Different behavior when interrupting a job, depending on the parallel flow order #3939

Closed

Fix unfinished step in parallel flow

b8a1ba8

doontagi force-pushed the fix-unfinished-step-in-parellel-flow branch from f101e58 to b8a1ba8 Compare March 24, 2024 14:23

doontagi commented Mar 27, 2024

View reviewed changes

fmbenhassine added pr-for: bug in: core labels Apr 15, 2024

fmbenhassine added the status: waiting-for-reporter Issues for which we are waiting for feedback from the reporter label Apr 23, 2024

fmbenhassine added status: feedback-provided Issues for which the feedback requested from the reporter was provided and removed status: waiting-for-reporter Issues for which we are waiting for feedback from the reporter labels Sep 2, 2024

fmbenhassine closed this Dec 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix unfinished step in parallel flow #4567

Fix unfinished step in parallel flow #4567

Uh oh!

doontagi commented Mar 24, 2024 •

edited

Loading

Uh oh!

doontagi Mar 27, 2024 •

edited

Loading

Uh oh!

doontagi Mar 27, 2024 •

edited

Loading

Uh oh!

doontagi commented Mar 27, 2024

Uh oh!

fmbenhassine commented Apr 23, 2024

Uh oh!

doontagi commented Jul 14, 2024 •

edited

Loading

Uh oh!

doontagi commented Dec 16, 2024

Uh oh!

fmbenhassine commented Dec 17, 2024

Uh oh!

fmbenhassine commented Dec 17, 2024

Uh oh!

Uh oh!

Fix unfinished step in parallel flow #4567

Fix unfinished step in parallel flow #4567

Uh oh!

Conversation

doontagi commented Mar 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Modification

Result

Uh oh!

doontagi Mar 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

doontagi Mar 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

doontagi commented Mar 27, 2024

Uh oh!

fmbenhassine commented Apr 23, 2024

Uh oh!

doontagi commented Jul 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

doontagi commented Dec 16, 2024

Uh oh!

fmbenhassine commented Dec 17, 2024

Uh oh!

fmbenhassine commented Dec 17, 2024

Uh oh!

Uh oh!

doontagi commented Mar 24, 2024 •

edited

Loading

doontagi Mar 27, 2024 •

edited

Loading

doontagi Mar 27, 2024 •

edited

Loading

doontagi commented Jul 14, 2024 •

edited

Loading